Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support granular UI annotations and labels #132

Merged
merged 4 commits into from
Dec 6, 2023
Merged

Conversation

woz5999
Copy link
Contributor

@woz5999 woz5999 commented Nov 20, 2023

This PR adds the ability to specifically target the UI pods with annotations. The current available annotations configurations apply to multiple deployments, including the UI pods. In order to expose the service via our service mesh, we need the ability to target the UI pods with annotations. This change allows us to inject and expose the UI pods only, without inadvertently annotated undesired pods.

if you need me to make any changes like bumping versions or updating changelogs, etc, just let me know and I'm happy to do so.

@bdjohnson529
Copy link
Contributor

@woz5999 Are the UI pods the pods within the backend deployment? Those pods would be:

  • MAIN_BACKEND
  • DB_CONNECTOR
  • DB_SSH_CONNECTOR
  • JOBS_RUNNER

I can see that the backend annotation is shared with the workflows and workflows worker deployments.

Is your primary concern having a separate annotation which exclusively targets pods within the backend deployment?

@bdjohnson529 bdjohnson529 requested a review from suyashcb December 5, 2023 13:52
@woz5999
Copy link
Contributor Author

woz5999 commented Dec 5, 2023

@bdjohnson529 That's correct. Our service mesh leverages annotations for registration and routing. Currently, if we set the annotation for the "backend" is applies to the workflow pods as well, meaning that UI requests regularly get round robin'ed to non-UI pods, resulting in HTTP errors. By being able to target the UI pods individually, the issue is totally resolved.

@jjlgao
Copy link
Contributor

jjlgao commented Dec 6, 2023

Hey @woz5999 , Jason from Retool's infra team -- the PR makes sense to me, but we don't internally call these non-workflows pods "ui", so checking with our team on what we'd like that naming to be.

@woz5999
Copy link
Contributor Author

woz5999 commented Dec 6, 2023

Sure thing. Naming's hard. Just let me know when you're ready and I can happily update.

@jjlgao
Copy link
Contributor

jjlgao commented Dec 6, 2023

Hey @woz5999 , we talked internally and we're good with using ui, but I just realized that we have a requirement for the two values.yaml files to match in the repo for legacy reasons. There's this topmost values.yaml file if you could copy your change over as well: https://github.com/tryretool/retool-helm/blob/main/values.yaml

Once you do that I'll merge and release this change for you.

Thanks!

@woz5999
Copy link
Contributor Author

woz5999 commented Dec 6, 2023

@jjlgao done. let me know if you need anything else

@woz5999
Copy link
Contributor Author

woz5999 commented Dec 6, 2023

The linter is failing because the chart version needs to be updated. I'm happy to do that.
✖︎ retool => (version: "6.0.8", path: "charts/retool") > chart version not ok. Needs a version bump!

I'm guessing 6.1.0 should be the new version?

@jjlgao
Copy link
Contributor

jjlgao commented Dec 6, 2023

The linter is failing because the chart version needs to be updated. I'm happy to do that. ✖︎ retool => (version: "6.0.8", path: "charts/retool") > chart version not ok. Needs a version bump!

I'm guessing 6.1.0 should be the new version?

6.0.9, thanks!

@woz5999
Copy link
Contributor Author

woz5999 commented Dec 6, 2023

Done

@jjlgao jjlgao merged commit 7115338 into tryretool:main Dec 6, 2023
12 checks passed
@woz5999
Copy link
Contributor Author

woz5999 commented Dec 6, 2023

Thanks everyone. When do new releases get published to your helm repository?

@jjlgao
Copy link
Contributor

jjlgao commented Dec 6, 2023

They're done manually. I'm going to publish it in a bit.

@jjlgao
Copy link
Contributor

jjlgao commented Dec 6, 2023

Just published, try pulling now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants